Skip to content

[breaking] uniform cli commands and flag #1542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 23 commits into from
Nov 19, 2021
Merged

[breaking] uniform cli commands and flag #1542

merged 23 commits into from
Nov 19, 2021

Conversation

umbynos
Copy link
Contributor

@umbynos umbynos commented Nov 9, 2021 •
edited
Loading

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?

refactor CLI commands and flags to be consistent with each other.

  • What is the current behavior?

close #1508

  • What is the new behavior?

This PR includes various improvements:

  • sketchpath calculation have been factored out

  • fqbn and programmer flags now have the same principle of operation as port flag

  • factored out a function that reports errors when conflicting flags are used

  • move vars on top and remove structs for uniformity

  • part of the init section in update-index commands have been factored out

  • other minor improvements

  • I also added logrus.Info() in every run function of every command

  • remove board details <fqbn> in favour of board details -b <fqbn>

  • replace board attach <port|fqbn> <sketch-path> with board attach -b <fqbn> | -p <port> <sketh-path>

  • change --timeout flag to --discovery-timeout in board list command for consistency

  • Does this PR introduce a breaking change, and is
    titled accordingly?

yep, documented in UPGRADING.md

  • Other information:

See how to contribute

@umbynos umbynos added type: enhancement Proposed improvement topic: code Related to content of the project itself topic: CLI Related to the command line interface labels Nov 9, 2021
@umbynos umbynos self-assigned this Nov 9, 2021
@umbynos umbynos requested a review from a team November 9, 2021 16:41
@umbynos umbynos force-pushed the umbynos/flags_refactoring branch from ba5385f to 66dbc61 Compare November 10, 2021 14:04
per1234
per1234 requested changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a big PR that contains multiple atomic commits, I recommend it be granted an exemption from the repo's usual squash merge policy.

This would be done by temporarily adjusting the configuration of the repo's merge button, following this procedure:

  1. Click the "⚙️ Settings" tab of the repository that contains the PR
  2. Under the "Merge button" section of the Settings page, check the box next to "🔲 Allow merge commits"
  3. Merge the PR, selecting the "Merge pull request" option from the merge button
  4. Return to the repository's Settings page
  5. Uncheck the box next to "🔲 Allow merge commits"

Since the commits will not be automatically squashed, any fixup commits should be manually squashed via an interactive rebase before the merge.

@umbynos umbynos force-pushed the umbynos/flags_refactoring branch 4 times, most recently from 2edafc0 to 80be710 Compare November 11, 2021 15:00
@umbynos umbynos changed the title uniform cli commands and flag [breaking] uniform cli commands and flag Nov 11, 2021
@umbynos umbynos force-pushed the umbynos/flags_refactoring branch from 4d2df31 to 8713d91 Compare November 11, 2021 15:41
per1234
per1234 requested changes Nov 11, 2021
View reviewed changes
per1234
per1234 approved these changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff Umberto. Thanks!

silvanocerza
silvanocerza reviewed Nov 17, 2021
View reviewed changes
silvanocerza
silvanocerza reviewed Nov 17, 2021
View reviewed changes
silvanocerza
silvanocerza reviewed Nov 17, 2021
View reviewed changes
silvanocerza
silvanocerza reviewed Nov 17, 2021
View reviewed changes
silvanocerza
silvanocerza suggested changes Nov 17, 2021
View reviewed changes
Copy link
Contributor

@silvanocerza silvanocerza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

@umbynos umbynos force-pushed the umbynos/flags_refactoring branch 2 times, most recently from fb73cf8 to 61dba74 Compare November 18, 2021 14:47
@umbynos umbynos requested a review from silvanocerza November 18, 2021 14:50
@umbynos umbynos force-pushed the umbynos/flags_refactoring branch from 61dba74 to e092b26 Compare November 19, 2021 10:35
umbynos added 17 commits November 19, 2021 12:32
@umbynos
refactor sketch path calculation
be5022e
@umbynos
refactor config validation to remove some code duplication
d1a41f8
@umbynos
refactor function that checks for flags conflicts
47821ae
@umbynos
refactor and reorganize core commands:
ea0334c 
move `DetectSkipPostInstallValue` in arguments package
create `PostInstallFlags` struct in arguments package
remove `listFlags` struct
@umbynos
refactor upload command to use code from the arguments package
9131edb
@umbynos
move vars on top (like other files)
8394280
@umbynos
refactor debug command and use code from arguments, remove useless vars
06e96b9
@umbynos
factor out init step for instance creation in updatecommands
4428a46
@umbynos
refactor lib command and use code from arguments, remove structs
0f25964
@umbynos
refactor monitor command to be consistent with other commands
aeb1528
@umbynos
general improvements for consistency between commands
8ff498b
@umbynos
remove useless break statements
e642f72
@umbynos
added info logging in every command for consistency
c316b38
@umbynos
[breaking] remove board details <fqbn> in favour of `board details …
633c177 
…-b <fqbn>`
@umbynos
[breaking] replace board attach <port|fqbn> <sketch-path> with `boa…
f85513c 
…rd attach -b <fqbn> | -p <port> <sketh-path>`
@umbynos
[breaking] change --timeout flag to --discovery-timeout for consi…
56add2e 
…stency
@umbynos
update UPGRADING.md with breaking changes
294c7bc
@umbynos umbynos force-pushed the umbynos/flags_refactoring branch from e092b26 to 294c7bc Compare November 19, 2021 11:34
@umbynos umbynos merged commit 9c13e87 into master Nov 19, 2021
@umbynos umbynos deleted the umbynos/flags_refactoring branch November 19, 2021 11:58
@umbynos umbynos mentioned this pull request Nov 19, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silvanocerza silvanocerza silvanocerza approved these changes

@per1234 per1234 per1234 approved these changes

Assignees

@umbynos umbynos

Labels
topic: CLI Related to the command line interface topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Factor CLI argument processing for flags that are common across commands
3 participants
@umbynos @silvanocerza @per1234